-
Notifications
You must be signed in to change notification settings - Fork 49
feat(rln)!: generate contract types, migrate from ethers to viem #2705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
1fcf108 to
a88dd8c
Compare
size-limit report 📦
|
264f2c0 to
5ea574e
Compare
70922e5 to
7e5680f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the RLN package from ethers.js to viem for Ethereum interactions and introduces automated contract type generation using wagmi CLI. The changes enable type-safe contract interactions by generating TypeScript bindings directly from the smart contract ABIs.
Key changes:
- Replaced ethers.js with viem for all blockchain interactions
- Automated ABI generation via wagmi CLI from waku-rlnv2-contract repository
- Updated all contract interaction code to use viem's type-safe APIs
- Added new methods for querying merkle root and merkle proof directly from contracts
Reviewed Changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rln/wagmi.config.ts | Configures wagmi CLI to generate TypeScript bindings from Foundry contracts |
| packages/rln/src/contract/wagmi/generated.ts | Auto-generated type-safe contract ABIs (977 lines) |
| packages/rln/src/contract/rln_base_contract.ts | Migrated from ethers Contract to viem getContract with type-safe reads/writes |
| packages/rln/src/utils/walletClient.ts | New utility to create viem client from window.ethereum |
| packages/rln/src/types.ts | Updated to use viem WalletClient instead of ethers.Signer |
| packages/rln/generate_contract_abi.sh | Shell script to clone, build, and generate contract types |
| packages/rln/package.json | Removed ethers dependency, added viem and wagmi tooling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fromBlock = options.fromBlock | ||
| ? BigInt(options.fromBlock!) | ||
| : BigInt(this.deployBlock!); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant non-null assertion after already checking with ternary operator. Line 195 uses options.fromBlock! after already checking options.fromBlock in the ternary condition. The non-null assertion is unnecessary here. Additionally, line 196 uses this.deployBlock! which could be undefined if the create() method hasn't been awaited, leading to a potential runtime error.
| const fromBlock = options.fromBlock | |
| ? BigInt(options.fromBlock!) | |
| : BigInt(this.deployBlock!); | |
| let fromBlock: bigint; | |
| if (options.fromBlock !== undefined) { | |
| fromBlock = BigInt(options.fromBlock); | |
| } else { | |
| if (this.deployBlock === undefined) { | |
| throw new Error("deployBlock is undefined. Ensure the contract is initialized and create() has been awaited before calling fetchMembers."); | |
| } | |
| fromBlock = BigInt(this.deployBlock); | |
| } |
| const registeredMemberEvents = | ||
| await this.contract.getEvents.MembershipRegistered({ | ||
| fromBlock, | ||
| toBlock: fromBlock + BigInt(options.fetchRange!) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing null check for options.fetchRange. Lines 200, 204, and 209 use options.fetchRange! with non-null assertion, but there's no guarantee this value exists. This will cause undefined to be converted to NaN when passed to BigInt(), resulting in a runtime error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reading it again, we don't actually need these events tracked with the new version of RLN smart contracts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we will, but I will re-implement when doing #2722
| return undefined; | ||
| } | ||
| private async getMemberIndex(idCommitmentBigInt: bigint): Promise<number> { | ||
| return (await this.contract.read.memberships([idCommitmentBigInt]))[5]; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number index [5] without explanation. Line 652 accesses array element at index 5 without documentation. According to the ABI at lines 573-584 in generated.ts, index 5 corresponds to the 'index' field of the membership struct. Consider adding a comment or using destructuring with named variables for clarity.
| return (await this.contract.read.memberships([idCommitmentBigInt]))[5]; | |
| // According to the ABI, index 5 corresponds to the 'index' field of the membership struct | |
| const [, , , , , index] = await this.contract.read.memberships([idCommitmentBigInt]); | |
| return index; |
packages/rln/tsconfig.json
Outdated
| "include": ["src"], | ||
| "exclude": ["src/**/*.spec.ts", "src/test_utils"] | ||
| } | ||
| "exclude": ["wagmi.config.ts"] |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed exclusion of test files and test_utils. The original exclusion pattern [\"src/**/*.spec.ts\", \"src/test_utils\"] was removed and replaced with only [\"wagmi.config.ts\"]. This means test files and test utilities will now be included in the build output, which is likely unintended and could increase bundle size.
| "exclude": ["wagmi.config.ts"] | |
| "exclude": ["wagmi.config.ts", "src/**/*.spec.ts", "src/test_utils"] |
| return; | ||
| } | ||
|
|
||
| const blockNumber = Number(evt.blockNumber); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting blockNumber to Number repeatedly in loop. Line 229 converts evt.blockNumber to Number inside the forEach loop (lines 225-256). If the block number exceeds JavaScript's MAX_SAFE_INTEGER (2^53-1), this conversion will lose precision. Consider keeping as bigint for accuracy or add a check for safe integer range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current block number on linea sepolia is 20646374 so this shouldn't be an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Withdraw deposited tokens after membership is erased | ||
| * @param token - Token address to withdraw | ||
| * NOTE: Funds are sent to msg.sender (the walletClient's address) |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 325 mentions that "Funds are sent to msg.sender (the walletClient's address)", but the function signature no longer includes a walletAddress parameter. This is a breaking API change that should be clearly documented. The previous implementation accepted walletAddress as a parameter but now it implicitly uses the wallet client's address. Consider noting this in the PR description or adding a migration guide.
| * Withdraw deposited tokens after membership is erased | |
| * @param token - Token address to withdraw | |
| * NOTE: Funds are sent to msg.sender (the walletClient's address) | |
| * Withdraw deposited tokens after membership is erased. | |
| * @param token - Token address to withdraw | |
| * @note Funds are sent to msg.sender (the walletClient's address). | |
| * @breaking The `walletAddress` parameter has been removed. This function now always sends funds to the wallet client's address (`msg.sender`). |
packages/rln/README.md
Outdated
| We use `wagmi` to generate TypeScript bindings for interacting with the RLN smart contracts. When changes are pushed to the `waku-rlnv2-contract` repository, run the following script to fetch and build the latest contracts and generate the TypeScript bindings: | ||
|
|
||
| ``` | ||
| ./generate_contract_abi.sh |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script name in package.json is setup:contract-abi but the README refers to ./generate_contract_abi.sh. The npm script should be referenced instead for consistency: npm run setup:contract-abi
| ./generate_contract_abi.sh | |
| npm run setup:contract-abi |
packages/rln/package.json
Outdated
| "prepublish": "npm run build", | ||
| "reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build" | ||
| "reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build", | ||
| "setup:contract-abi": "./setup-contract-abi.sh", |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file path in the package.json script is ./setup-contract-abi.sh, but the actual file is named generate_contract_abi.sh. This mismatch will cause the script to fail when executed.
| "setup:contract-abi": "./setup-contract-abi.sh", | |
| "setup:contract-abi": "./generate_contract_abi.sh", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you robot
| ); | ||
| } | ||
|
|
||
| const [account] = await ethereum.request({ method: "eth_requestAccounts" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not safe if .request doesn't resolve into array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like viem has a fully-typed polyfill for window which includes all the rpc requests, replacing with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the fix? I think I still see old code, @adklempner
|
|
||
| export const RLN_CONTRACT = { | ||
| chainId: 59141, | ||
| address: "0xb9cd878c90e49f797b4431fbf4fb333108cb90e6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get these addresses from some other place similarly to abi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately no; we can move this elsewhere but this is a value that needs to be manually set for now.
packages/rln/package.json
Outdated
| "reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build" | ||
| "reset-hard": "git clean -dfx -e .idea && git reset --hard && npm i && npm run build", | ||
| "setup:contract-abi": "./setup-contract-abi.sh", | ||
| "generate:contract": "npx wagmi generate" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
questions:
- this command is not needed - if we have previous
setup:contract-abi - how is
setup:contract-abisupposed to run? before commit / build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- yes we can remove this one
- it only needs to be run if the smart contracts are updated and we need to re-generate typings
| @@ -0,0 +1,42 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is not cross platform and will not work on Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/search?q=org%3Awaku-org+%23%21%2Fbin%2Fbash&type=code
¯_(ツ)_/¯
also, I can't imagine someone trying to make contributions to our repos without WSL, which should enable running bash...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯_(ツ)_/¯
there is no js-waku related script that are needed for contribution
I can't imagine someone trying to make contributions to our repos without WSL
and what exactly requiring WSL?
weboko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good first iteration
left comments in addition to Copilot's
and make sure you properly integrate it with release CI
93c152a to
e0c6a09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
44ad556 to
8221064
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| instance.maxRateLimit = ethers.BigNumber.from(max).toNumber(); | ||
|
|
||
| instance.minRateLimit = min; | ||
| instance.maxRateLimit = max; |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type assertion address! as \0x${string}`` uses the non-null assertion operator which could throw at runtime if address is undefined. Consider adding a runtime check instead:
if (!address) {
throw new Error("Contract address is required");
}| } | ||
| ); | ||
| } catch (err) { | ||
| throw new Error("Failed to simulate register membership: " + err); |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another error message formatting issue. Consider using proper error formatting:
throw new Error(`Failed to simulate register membership: ${err instanceof Error ? err.message : String(err)}`);| throw new Error("Failed to simulate register membership: " + err); | |
| throw new Error( | |
| `Failed to simulate register membership: ${err instanceof Error ? err.message : String(err)}` | |
| ); |
| * and will only send tokens to that address. | ||
| * @param token - Token address to withdraw | ||
| */ | ||
| public async withdraw(token: string): Promise<Hash> { |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The withdraw method signature has changed - it no longer requires walletAddress parameter. This is a breaking change. The documentation comment explains this is intentional (the smart contract validates the sender), but consider whether the removal of this parameter might confuse users who were previously required to provide it. The comment at line 323 correctly documents the new behavior.
|
|
||
| export interface MembershipInfo { | ||
| index: ethers.BigNumber; | ||
| index: number; |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The index type has changed from ethers.BigNumber to number. This is a breaking change that could cause issues if index values exceed JavaScript's MAX_SAFE_INTEGER (2^53 - 1). Consider whether membership indices could realistically exceed this limit, and if so, keep using bigint instead.
| index: number; | |
| index: bigint; |
| } | ||
|
|
||
| return { | ||
| index, |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The index field is assigned directly from the contract without type conversion, but the MembershipInfo type expects index: number. Since the contract returns uint32, this should be safe, but for consistency with other conversions in this function, consider explicitly converting:
index: Number(index),| index, | |
| index: Number(index), |
| } | ||
| ); | ||
| } catch (err) { | ||
| throw new Error("Error simulating eraseMemberships: " + err); |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar error message formatting issue. Consider using proper error formatting:
throw new Error(`Error simulating eraseMemberships: ${err instanceof Error ? err.message : String(err)}`);| throw new Error("Error simulating eraseMemberships: " + err); | |
| throw new Error(`Error simulating eraseMemberships: ${err instanceof Error ? err.message : String(err)}`); |
| account: this.rpcClient.account.address | ||
| }); | ||
| } catch (err) { | ||
| throw new Error("Error simulating withdraw: " + err); |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another error message formatting issue. Consider using proper error formatting:
throw new Error(`Error simulating withdraw: ${err instanceof Error ? err.message : String(err)}`);| throw new Error("Error simulating withdraw: " + err); | |
| throw new Error(`Error simulating withdraw: ${err instanceof Error ? err.message : String(err)}`); |
|
from previous review one point is not addressed
I think we should:
|
weboko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
please, address other comments before merge
Problem / Description
Previous to this, the ABIs for the contracts were manually added to the rln package (not clear how).
Additionally, code related to calling the contracts was not typed, so any breaking changes to the ABI would not be detected at compile time.
Solution
https://github.com/waku-org/waku-rlnv2-contractThe rest of the code has been updated to use viem, which provides clean and typesafe ways to interact with the smart contracts.
Also adds new methods that were introduced in latest contract changes, specifically the ability to query for merkle root and merkle proof for a commitment directly from the smart contract.
Notes
Checklist